Skip to content

feat(mail): drop the Pub/Sub requirement — scheduled fetch as primary intake (HT-94) - #107

Merged
zaridan merged 3 commits into
mainfrom
feat/ht-94-scheduled-fetch-primary
Jul 25, 2026
Merged

feat(mail): drop the Pub/Sub requirement — scheduled fetch as primary intake (HT-94)#107
zaridan merged 3 commits into
mainfrom
feat/ht-94-scheduled-fetch-primary

Conversation

@zaridan

@zaridan zaridan commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Stacked on #102 (the charter amendment this implements). Base is docs/ht-92-scheduled-fetch-intake, not main — review them together, since #102 is what makes this permissible under §2.

Why

About half the ~26-step setup exists to make Gmail push work, and two of those steps fail silently: the domain-restricted-sharing org policy blocking the Pub/Sub IAM grant, and a CLI-created subscription missing roles/iam.serviceAccountTokenCreator. Pub/Sub also forces billing on the Cloud project; the Gmail API alone does not.

What changes

Push becomes optional. The three GMAIL_PUBSUB*/GMAIL_PUSH* vars become one optional object rather than three optional strings, so half-configured push is unrepresentable. A partial config is a boot error naming the missing vars — an operator who set a topic and forgot the service account has a broken push they believe works, which is the exact failure this removes.

Connect works with no topic. Step 4 skips the watch() arm and seeds the baseline cursor from getProfile(). That's safe for precisely the reason gmail-connect.md:28-31 rejects it in the push case — getProfile's historyId "could straddle the arm," and with no arm there's nothing to straddle. No extra API call either: step 3 already calls getProfile for the address.

The sweep becomes the primary transport, moved to src/mail/gmail-reconcile-sweep.ts on an every-minute cron.

The two findings that shaped this

Reconciliation already works this way. gmail-reconcile.ts reads the mailbox's stored cursor, explicitly never the push notification's historyId. Push only makes the same job run sooner. So the sweep enqueues the identical GMAIL_RECONCILE_TOPIC job the webhook does — a push-free deployment ingests through exactly the code path a push deployment does. That equivalence is what makes this safe under the mail-semantics invariant.

The sweep needs no access token — it reads a cursor and enqueues. Watch renewal needs one because it calls users.watch(). Keeping them welded would have meant a token refresh per mailbox per minute against Google's token endpoint for a call the sweep never makes. That's why this is a module split rather than a flag.

No migration

gmail_watch_state.watch_expiration was already nullable — only the TypeScript signature was stricter than the schema. Verified against the live database.

Verification

Check Result
tsc --noEmit exit 0
biome check . exit 0
npm test exit 01517 tests, 76 files

Reviewer attention

  • dedupeKey is still absent, deliberately. At every-minute cadence the HT-48 consumer lease stops being an efficiency guard and becomes structural — ticks will overlap a running reconcile on a busy mailbox, and the lease is what makes that a no-op instead of duplicated fetching. Worth confirming you agree that reasoning holds at 60× the old rate.
  • Cost. One extra */1 cron. Estimated well under a dollar a month in compute; the sweep is the cheapest of the five since it makes no external call. I could not retrieve Vercel's rate card to confirm the figure — the magnitude (~1.8 GB-hours/month) is solid, the dollars are an estimate.
  • watch-maintenance stays routed when push is off and reports a skip rather than 404-ing, because vercel.json is static and a daily not-found would read like a fault.

Also

Runbook Part A now leads with A3/A4 being optional and why skipping is recommended, and its cron list is corrected from a stale "three" to the actual five — closing one of the doc-drift defects found while mapping the setup path.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added a scheduled reconciliation sweep to help process inbound Gmail messages reliably.
    • Gmail push notifications are now optional; deployments without push use scheduled reconciliation automatically.
    • Gmail connection setup skips watch registration when push is not configured.
  • Bug Fixes

    • Prevented duplicate reconciliation jobs while allowing retries after completion or failure.
    • Improved per-mailbox failure handling so one mailbox does not block others.
  • Documentation

    • Updated deployment, configuration, monitoring, alerting, and operational guidance for optional push and scheduled reconciliation.
    • Added validation guidance for incomplete push configuration.

@coderabbitai

coderabbitai Bot commented Jul 20, 2026

Copy link
Copy Markdown

Review Change Stack

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 564659d1-32b0-48ad-884c-26a9a4f7cf37

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Gmail inbound processing now treats Pub/Sub push as optional, adds an independent per-minute reconciliation sweep, limits watch maintenance to push deployments, and updates connection, configuration, health, cron routing, tests, deployment scheduling, and operational guidance.

Changes

Gmail inbound flow

Layer / File(s) Summary
Optional push configuration and connect behavior
src/composition/config.ts, src/composition/config.test.ts, src/mail/gmail-connect.ts, src/store/gmail-watch-state.ts
Gmail push settings are represented as an optional all-or-nothing object. Connect skips watch() without push and seeds the profile cursor; baseline persistence accepts a missing expiration.
Scheduled reconciliation sweep
src/mail/gmail-reconcile-sweep.ts, src/mail/gmail-reconcile-sweep.test.ts
Active mailboxes with cursors produce deduplicated reconcile jobs, cursor-less mailboxes are skipped, and per-mailbox failures are reported without stopping the sweep.
Cron routing and maintenance split
src/mail/gmail-watch-maintenance.ts, src/composition/root.ts, src/composition/app.ts, src/composition/app.test.ts, vercel.json
Watch maintenance now renews watches only. A separate authenticated reconcile-sweep route is wired and scheduled every minute.
Push-aware health and operations
src/composition/health.ts, src/composition/health.test.ts, specs/deploy/gmail-inbound-runbook.md
Watch-expiring alerts are gated by push configuration, and deployment, alerting, logging, and optional push setup guidance are updated.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Cron as reconcile-sweep cron
  participant Sweep as runGmailReconcileSweep
  participant Mailboxes as MailboxStore
  participant State as GmailWatchStateStore
  participant Queue as QueueProvider
  Cron->>Sweep: invoke sweep
  Sweep->>Mailboxes: listActiveMailboxes()
  loop active mailboxes
    Sweep->>State: getCursor(mailboxId)
    Sweep->>Queue: enqueue reconcile job
  end
  Sweep-->>Cron: return sweep report
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately captures the main change: making Pub/Sub optional and using scheduled reconciliation as the primary intake path.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/ht-94-scheduled-fetch-primary

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@zaridan
zaridan force-pushed the docs/ht-92-scheduled-fetch-intake branch from a5bd06c to 7e76182 Compare July 21, 2026 16:30
Base automatically changed from docs/ht-92-scheduled-fetch-intake to main July 21, 2026 17:01
@zaridan
zaridan force-pushed the feat/ht-94-scheduled-fetch-primary branch from aa89eb9 to 2c0e528 Compare July 21, 2026 17:22

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
specs/deploy/gmail-inbound-runbook.md (1)

363-364: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Alert table reuses existing codes as a new compound row — confusing lookup.

Row 364 repeats the queue-drain-stalled / queue-dead-letter-growth codes already defined at line 358-359 as if they were a distinct table entry, just with "(on a push-free deployment)" appended to the key. Since G2 states each alerts[] entry is one stable <code>: <detail> pair, an operator scanning this table mid-incident could reasonably wonder whether this is a third, different code rather than push-free-specific guidance for the same two codes.

Consider folding this into the existing rows (e.g., append the push-free implication as an extra sentence in the original queue-drain-stalled/queue-dead-letter-growth "First response" cells) rather than a separate table row with a compound key.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@specs/deploy/gmail-inbound-runbook.md` around lines 363 - 364, Remove the
separate push-free `queue-drain-stalled` / `queue-dead-letter-growth` row from
the alert table. Fold its push-free deployment implication and troubleshooting
guidance into the existing rows for those stable alert codes, preserving one
code-to-detail entry per `alerts[]` item and avoiding a compound key.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@specs/deploy/gmail-inbound-runbook.md`:
- Around line 363-364: Remove the separate push-free `queue-drain-stalled` /
`queue-dead-letter-growth` row from the alert table. Fold its push-free
deployment implication and troubleshooting guidance into the existing rows for
those stable alert codes, preserving one code-to-detail entry per `alerts[]`
item and avoiding a compound key.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 0825891d-f46e-4a7b-bfbd-cb1fc4e23865

📥 Commits

Reviewing files that changed from the base of the PR and between e663e4d and 2c0e528.

📒 Files selected for processing (16)
  • specs/deploy/gmail-inbound-runbook.md
  • src/composition/app.test.ts
  • src/composition/app.ts
  • src/composition/config.test.ts
  • src/composition/config.ts
  • src/composition/health.test.ts
  • src/composition/health.ts
  • src/composition/root.test.ts
  • src/composition/root.ts
  • src/mail/gmail-connect.ts
  • src/mail/gmail-reconcile-sweep.test.ts
  • src/mail/gmail-reconcile-sweep.ts
  • src/mail/gmail-watch-maintenance.test.ts
  • src/mail/gmail-watch-maintenance.ts
  • src/store/gmail-watch-state.ts
  • vercel.json

zaridan and others added 3 commits July 25, 2026 08:23
…HT-94)

First half of dropping the Pub/Sub requirement. The engine previously
refused to boot without GMAIL_PUBSUB_TOPIC, GMAIL_PUBSUB_SUBSCRIPTION and
GMAIL_PUSH_SERVICE_ACCOUNT — three vars whose provisioning is the majority
of the Google Cloud setup burden, and the half that fails silently (the
domain-restricted-sharing org-policy block, and the missing
serviceAccountTokenCreator grant).

Config: the three become ONE optional object rather than three optional
strings, so a half-configured push is unrepresentable — you cannot arm
watch() against a topic without also being able to authenticate the
resulting push. A partial config is a boot error naming the missing vars,
never a silent fallback to "push off": an operator who set a topic and
forgot the service account has a broken push they believe works, which is
the precise failure this work exists to remove.

Connect: with no topic, step 4 skips the watch() arm entirely and seeds
the baseline cursor from getProfile(). That substitution is safe for
exactly the reason gmail-connect.md gives for rejecting it in the push
case — getProfile's separately-read historyId "could straddle the arm,"
and with no arm there is nothing to straddle. No extra API call: step 3
already calls getProfile to resolve the mailbox address, and its response
carries the historyId.

Store: seedBaseline's watchExpiration becomes optional. No migration —
gmail_watch_state.watch_expiration is already nullable; only the
TypeScript signature was stricter than the schema.

Root: the push webhook deps are built ONLY when push is configured, so an
endpoint that cannot verify what it receives is never routable. The
watch-maintenance cron stays routed but reports a skip, because
vercel.json is static and a daily 404 would read like a fault.

Verified: tsc exit 0, biome exit 0, 1512 tests pass across 75 files.

NOT yet done — the sweep still runs daily, so a push-free deployment would
only ingest once a day. Extracting it to its own every-minute cron is the
next commit and is what makes this coherent. Worth noting the sweep needs
no access token (it reads a cursor and enqueues), unlike the watch re-arm
it is currently welded to — which matters at 1/min cadence.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…-94)

Second half of dropping the Pub/Sub requirement. The first commit made push
optional; without this one a push-free deployment would ingest mail once a
day, because the reconciliation sweep was welded to the daily
watch-maintenance cron.

The sweep moves to src/mail/gmail-reconcile-sweep.ts and runs every minute
as its own cron. Two reasons it became a separate entry point rather than a
flag on the existing one:

- Cadence. As a push backstop it ran daily; as the primary transport it
  runs every minute. Those cannot share a cron.
- Cost, and this is the load-bearing one. Watch renewal must acquire an
  access token per mailbox because it calls users.watch(). The sweep must
  not — it reads a stored cursor and enqueues, making no Gmail call at all.
  Keeping them welded would have meant a token refresh per mailbox per
  MINUTE against Google's token endpoint, for a call the sweep never makes.
  The reconcile consumer acquires its own token when it actually talks to
  Gmail.

Nothing about reconciliation itself changes: the sweep enqueues the SAME
GMAIL_RECONCILE_TOPIC job the push webhook enqueues, consumed by the same
handler. A deployment without Pub/Sub ingests through exactly the code path
a deployment with it does, triggered by a clock instead of a notification.
That equivalence is what makes this safe under the mail-semantics
invariant.

Enqueues still carry no dedupeKey. At every-minute cadence the consumer's
lease (HT-48) stops being an efficiency guard and becomes structural: ticks
WILL overlap a still-running reconcile on a busy mailbox, and the lease is
what makes that a no-op rather than duplicated fetching.

watch-maintenance is now renewal-only, still daily, and its module doc no
longer describes behavior that moved. The now-dead queue dependency is
removed from its deps rather than left unused.

Runbook: Part A leads with A3/A4 being optional and why skipping them is
recommended — six fewer steps, no billing requirement, and neither of the
two silent-failure traps. The cron list is corrected from a stale "three"
to the actual five, which also closes one of the doc-drift defects found
while mapping the current setup path.

Verified: tsc exit 0, biome exit 0, 1517 tests pass across 76 files.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e (HT-94)

Three independent reviews (two Opus-tier, one Codex) attacked this PR.
Two of the three claims I asked them to scrutinize did not survive.

**Health returned 503 permanently on the install path the runbook now
recommends.** health.ts alerts `watch-expiring` when an active mailbox has
no watch() expiration — which is the DESIGNED steady state once push is
optional. Since the endpoint's contract is a single boolean, that permanent
false alarm made every real alert invisible. runHealthCheck now takes
`pushConfigured` and gates the watch alerts on it. Deliberately a config
fact, not inferred from data: a NULL expiration means "no push configured"
on one deployment and "renewal cron is broken" on another, and only the
config distinguishes them. The gate is scoped to watch alerts —
mailbox-needs-attention still fires either way, with a test proving it.

I verified the column was nullable and stopped there, never checking what
READS it.

**The sweep now enqueues with `dedupeKey: mailboxId`.** Carrying "no
dedupeKey" from a DAILY cadence to an every-minute one was the actual
mistake, and it was not benign:

- The consumer lease does not make contention free. A failed claim returns
  retry, the queue counts attempts, and jobs DEAD-LETTER at the cap. A
  reconcile outrunning the retry window made every tick behind it burn its
  attempts and dead-letter — tripping queue-dead-letter-growth, a second
  permanent 503. The lease prevents duplicated work, not duplicated rows.
- There was no backpressure at all. Enqueue was unconditional per mailbox
  per minute against a bounded drain batch shared with webhook delivery.

The original reasoning ("a quiet mailbox must still be swept") argues
against a COMPOSITE key, not the bare mailboxId: the partial unique index
only suppresses against still-live jobs, so a completed job unblocks the
next tick. Tests now prove that against the REAL Postgres queue over
PGlite, not a fake — a fake cannot exercise the index and would be
tautological. This also aligns the sweep with the push path, which has
always used a dedupe key.

**Coverage for the three headline branches**, all previously untested:
push-free health, resolveGmailPush (all-unset succeeds; every partial
throws naming the missing vars), and the new cron endpoint's routing and
CRON_SECRET enforcement.

**Runbook**: watch-expiring documented as push-only, and its remediation
corrected — a manual GET of watch-maintenance is a no-op on a push-free
deployment, so seeing that alert there is a bug, not a mailbox problem.
Added the two new log events and queue guidance for the push-free case.

**Corrections**: root.ts logged the maintenance skip under a different
event name than the module itself uses; the vestigial queue dependency and
its test helper are removed rather than left unused.

Not fixed, reported instead: my "exactly the same code path" claim was
overstated — push dedupes and (before this change) the sweep did not, and
`job.historyId` carries semantically opposite values depending on trigger.
Harmless while nothing reads it; noted in specs/mail/mailbox-connection.md.

One reviewer claim I overruled: Codex held that revoked OAuth grants would
go undetected without the old per-sweep token probe. listActiveMailboxes
excludes needs_reconnect at the SQL level and the consumer acquires a token
per job, so detection is now FASTER (~1h vs 24h) and a flagged mailbox
drops out of the sweep. What was lost is three counters — observability,
not correctness.

Verified: tsc exit 0, biome exit 0, 1531 tests pass across 76 files.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@zaridan
zaridan force-pushed the feat/ht-94-scheduled-fetch-primary branch from 2c0e528 to 328d2cb Compare July 25, 2026 15:33
@zaridan
zaridan merged commit f220f08 into main Jul 25, 2026
5 checks passed
@zaridan
zaridan deleted the feat/ht-94-scheduled-fetch-primary branch July 25, 2026 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant